Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FB-279: Sync FeatureBoard API state back to ExternalState provider if registered #79

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ickers
Copy link
Contributor

@ickers ickers commented Jan 11, 2024

Relevant JIRAs: FB-279 & FB-246

Summary of changes:

  • At Web App host startup, only attempt to load config from external state if fail to retrieve from FB API (FB-246)
  • Each time feature config changes (as determined by FB API ETag) write back to external state if registered (FB-279)
  • Add retry for 5xx, 408, 429 status code responses and transient network faults when calling FB API
    • Max 5 retries
    • Exponential backoff used unless Retry-After header is present in response

}
catch (ArgumentException e) // eg. thrown due to duplicate feature key
{
_logger.LogError(e, "Failed to update flags");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the eTag value be rolled back to its original value if exception occurs at this point (ie. during handling of the FeatureConfigurationUpdated event)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not hugely experienced in handling eTags, but isn't just using a new value fine since you are trying to compare tag values in the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the eTag retains its updated value and isn't rolled back then the code inside try block won't run again until FeatureBoard API responds back with a non 304 status (due to someone touching the feature config).

I think, at least for this exception scenario [caused by duplicate feature name appearing in FB API response), rollback is appropriate, so it keeps retrying the state update periodically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tricker question is what to do if IFeatureBoardExternalState throws

@ickers ickers force-pushed the bugfix/update-external-state-on-change-redux branch from e22d4c4 to 171826b Compare January 11, 2024 07:07
await scope.ServiceProvider.GetRequiredService<IFeatureBoardService>().RefreshFeatureConfiguration(cancellationToken);
using var scope = _scopeFactory.CreateScope();
var updated = await scope.ServiceProvider.GetRequiredService<IFeatureBoardService>().RefreshFeatureConfiguration(cancellationToken) ?? false;
if (updated || _externalState is null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah cool

@ickers ickers force-pushed the bugfix/update-external-state-on-change-redux branch from 171826b to f475ddc Compare January 11, 2024 07:35
@ickers ickers force-pushed the bugfix/update-external-state-on-change-redux branch 2 times, most recently from c4e6c63 to da09dd0 Compare January 16, 2024 01:49
…gnal as async exception behavhiour is not fit for purpose
@ickers ickers force-pushed the bugfix/update-external-state-on-change-redux branch from da09dd0 to fa68f9e Compare January 16, 2024 02:59
@ickers ickers marked this pull request as ready for review January 16, 2024 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants